Review Summary

Clean, well-scoped security hardening. The HttpOnly addition is correct, the doc comment is excellent, and the change is minimal. One concern about the debug_assert! approach and a few smaller items below.

0 Blockers · 1 High · 3 Medium

@prk-Jr sorry the previous review didn't have clear suggestions.

Solid, well-scoped security fix. The HttpOnly addition is correct and the doc comment is excellent. Three actionable findings — two about the sanitization approach, one about the assert!.

Approval

All findings from both review rounds have been addressed in the latest commit (2f3282f):

• Runtime safety: debug_assert! and per-request assert! replaced with allowlist sanitization + log::warn! on modification, and #[validate] on Publisher::cookie_domain for fail-at-startup config validation.
• Sanitization approach: Switched from denylist to allowlist (is_ascii_alphanumeric + . - _), which is more robust against unexpected metacharacters.
• Test coverage: Sanitization, passthrough, and validator tests all added.
• Documentation: Doc comments updated to reflect the new sanitization approach, with # Examples doctest included.

The follow-up to move sanitization upstream (to avoid the header/cookie value mismatch window) is tracked separately.

Looks good — nice work.

Summary

Adds HttpOnly to the synthetic ID cookie and introduces runtime allowlist sanitization of synthetic_id values plus config-time validation of cookie_domain. The core changes are correct, well-tested, and well-documented.

Non-blocking

♻️ refactor

• expire_synthetic_cookie missing HttpOnly (cookies.rs:204-210): For consistency with create_synthetic_cookie, the expiration cookie should also carry HttpOnly. Functionally correct without it — browsers match cookies by name+domain+path for deletion — but for consistency and defense-in-depth, the expiration cookie should match:

  "{}=; Domain={}; Path=/; Secure; HttpOnly; SameSite=Lax; Max-Age=0",

🤔 thinking

• Silent sanitization vs. rejection of tampered IDs (cookies.rs:167-178): When synthetic_id contains disallowed characters, the code strips them and logs a warning. The sanitized fragment (e.g. abc;injected → abcinjected) likely doesn't correspond to any real user state. An alternative: reject the tampered ID and generate a fresh one, since a synthetic ID with metacharacters is almost certainly an attack. This would also eliminate the header/cookie mismatch (next item).

• Header/cookie value mismatch on tampered IDs (publisher.rs:373-374): When the incoming x-synthetic-id header contains disallowed characters, the response x-synthetic-id header gets the raw value while the cookie gets the sanitized value. The log::warn documents this well, and the Fastly SDK prevents CRLF header injection. But downstream consumers reading header vs. cookie see different identities. If the "generate fresh on tampering" approach above is adopted, this mismatch goes away entirely.

🌱 seedling

• Cookie attribute helper (cookies.rs:162-183, cookies.rs:204-210): Both create_synthetic_cookie and expire_synthetic_cookie build Set-Cookie strings manually. As attributes accumulate, the risk of inconsistency grows (as the HttpOnly omission in expire_synthetic_cookie demonstrates). A future PR could extract the common attribute set into a helper so both functions share security properties by construction.

CI Status

• fmt: PASS
• clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• CodeQL: PASS